Resolve: refactor `define` into `define_local` and `define_extern` #143884

LorrensP-2158466 · 2025-07-13T11:08:38Z

Follow up on #143550 and part of #gsoc > Project: Parallel Macro Expansion.

Split up define into define_local and define_extern. Refactor usages of define into either one where it's "correct" (idk if everything is correct atm). Big part of this is that resolution can now take a &Resolver instead of a mutable one.

r? @petrochenkov

compiler/rustc_resolve/src/build_reduced_graph.rs

compiler/rustc_resolve/src/imports.rs

compiler/rustc_resolve/src/lib.rs

compiler/rustc_resolve/src/imports.rs

LorrensP-2158466 · 2025-07-13T20:35:22Z

Conflicts must be resolved in Petrochenkov's PRs, I think. I can then rebase off that.

petrochenkov · 2025-07-14T13:21:34Z

Blocked on #143550.
@rustbot blocked

compiler/rustc_resolve/src/build_reduced_graph.rs

resolve: Use interior mutability for extern module map Module map for extern modules is a lazily populated cache, it's not *significantly* mutable. If some logic in name resolver is parallelized, then this cache can be populated from any thread, and without affecting results of any speculative resolution. Unblocks rust-lang#143884. This is a part of [#gsoc > Project: Parallel Macro Expansion](https://rust-lang.zulipchat.com/#narrow/channel/421156-gsoc/topic/Project.3A.20Parallel.20Macro.20Expansion/with/527348747). cc `@LorrensP-2158466`

Rollup merge of #143550 - petrochenkov:lessmutres, r=lcnr resolve: Use interior mutability for extern module map Module map for extern modules is a lazily populated cache, it's not *significantly* mutable. If some logic in name resolver is parallelized, then this cache can be populated from any thread, and without affecting results of any speculative resolution. Unblocks #143884. This is a part of [#gsoc > Project: Parallel Macro Expansion](https://rust-lang.zulipchat.com/#narrow/channel/421156-gsoc/topic/Project.3A.20Parallel.20Macro.20Expansion/with/527348747). cc `@LorrensP-2158466`

LorrensP-2158466 · 2025-07-17T18:49:56Z

I couldn't find any more &mut Resolver that I could turn into &Resolver. Everything now either has finalize codepaths and/or lints/errors getting added to their respective collections.

resolve: Make disambiguators for underscore bindings module-local Disambiguators attached to underscore name bindings (like `const _: u8 = something;`) do not need to be globally unique, they just need to be unique inside the module in which they live, because the bindings in a module are basically kept as `Map<BindingKey, SomeData>`. Also, the specific values of the disambiguators are not important, so a glob import of `const _` may have a different disambiguator than the original `const _` itself. So in this PR the disambiguator is just set to the current number of bindings in the module. This removes one more piece of global mutable state from resolver and unblocks rust-lang#143884.

resolve: Use interior mutability for extern module map Module map for extern modules is a lazily populated cache, it's not *significantly* mutable. If some logic in name resolver is parallelized, then this cache can be populated from any thread, and without affecting results of any speculative resolution. Unblocks rust-lang/rust#143884. This is a part of [#gsoc > Project: Parallel Macro Expansion](https://rust-lang.zulipchat.com/#narrow/channel/421156-gsoc/topic/Project.3A.20Parallel.20Macro.20Expansion/with/527348747). cc `@LorrensP-2158466`

resolve: Make disambiguators for underscore bindings module-local Disambiguators attached to underscore name bindings (like `const _: u8 = something;`) do not need to be globally unique, they just need to be unique inside the module in which they live, because the bindings in a module are basically kept as `Map<BindingKey, SomeData>`. Also, the specific values of the disambiguators are not important, so a glob import of `const _` may have a different disambiguator than the original `const _` itself. So in this PR the disambiguator is just set to the current number of bindings in the module. This removes one more piece of global mutable state from resolver and unblocks rust-lang#143884.

Rollup merge of #144013 - petrochenkov:disambunder, r=oli-obk resolve: Make disambiguators for underscore bindings module-local Disambiguators attached to underscore name bindings (like `const _: u8 = something;`) do not need to be globally unique, they just need to be unique inside the module in which they live, because the bindings in a module are basically kept as `Map<BindingKey, SomeData>`. Also, the specific values of the disambiguators are not important, so a glob import of `const _` may have a different disambiguator than the original `const _` itself. So in this PR the disambiguator is just set to the current number of bindings in the module. This removes one more piece of global mutable state from resolver and unblocks #143884.

LorrensP-2158466 · 2025-07-24T13:35:10Z

Rebased and pushed, @rustbot ready.

compiler/rustc_resolve/src/lib.rs

compiler/rustc_resolve/src/imports.rs

compiler/rustc_resolve/src/build_reduced_graph.rs

LorrensP-2158466 · 2025-07-24T15:38:33Z

@rustbot ready

petrochenkov · 2025-07-25T03:23:15Z

compiler/rustc_resolve/src/build_reduced_graph.rs

        &mut self,
        parent: Module<'ra>,
        ident: Ident,
        ns: Namespace,
        res: Res,
-        vis: Visibility<impl Into<DefId>>,
+        vis: Visibility, // Implicitly local


Suggested change

vis: Visibility, // Implicitly local

vis: Visibility,

This comment is probably not very useful.

petrochenkov · 2025-07-25T03:25:04Z

compiler/rustc_resolve/src/build_reduced_graph.rs

+            parent.underscore_disambiguator.get()
+        });
+        let resolution = &mut *self.resolution(parent, key).borrow_mut();
+        if resolution.non_glob_binding.replace(binding).is_some() {


Suggested change

if resolution.non_glob_binding.replace(binding).is_some() {

if self.resolution(parent, key).borrow_mut().non_glob_binding.replace(binding).is_some() {

petrochenkov · 2025-07-25T03:25:44Z

@bors2 try @rust-timer queue

Resolve: refactor `define` into `define_local` and `define_extern`

rust-bors · 2025-07-25T03:25:48Z

⌛ Trying commit a9d94be with merge afc668a…

To cancel the try build, run the command @bors try cancel.

resolve: Make disambiguators for underscore bindings module-local (take 2) The difference with rust-lang/rust#144013 can be seen in the second commit. Now we just keep a separate disambiguator counter in every `Module`, instead of a global counter in `Resolver`. This will be ok for parallel import resolution because we'll need to lock the module anyway when updating `resolutions` and other fields in it. And for external modules the disabmiguator could be just passed as an argument to `define_extern`, without using any cells or locks, once rust-lang/rust#143884 lands. Unblocks rust-lang/rust#143884.

rust-bors · 2025-07-25T05:41:06Z

☀️ Try build successful (CI)
Build commit: afc668a (afc668a496c63580685da37f41e3e9130c09fffc, parent: b56aaec52bc0fa35591a872fb4aac81f606e265c)

LorrensP-2158466 · 2025-07-25T06:38:52Z

@rustbot ready

rust-timer · 2025-07-25T07:40:06Z

Finished benchmarking commit (afc668a): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.1%, 0.1%]	2
Regressions ❌ (secondary)	0.5%	[0.3%, 0.7%]	7
Improvements ✅ (primary)	-0.4%	[-0.7%, -0.3%]	13
Improvements ✅ (secondary)	-0.3%	[-0.6%, -0.2%]	23
All ❌✅ (primary)	-0.4%	[-0.7%, 0.1%]	15

Max RSS (memory usage)

Results (primary -0.8%, secondary -2.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.9%	[1.4%, 2.4%]	2
Regressions ❌ (secondary)	1.5%	[0.9%, 2.2%]	2
Improvements ✅ (primary)	-2.1%	[-2.3%, -2.0%]	4
Improvements ✅ (secondary)	-4.6%	[-6.9%, -2.1%]	4
All ❌✅ (primary)	-0.8%	[-2.3%, 2.4%]	6

Cycles

Results (secondary 3.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.4%	[2.5%, 4.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 469.891s -> 468.109s (-0.38%)
Artifact size: 374.63 MiB -> 374.66 MiB (0.01%)

petrochenkov · 2025-07-25T08:57:21Z

compiler/rustc_resolve/src/build_reduced_graph.rs

+        assert!(res.opt_def_id().is_none_or(|def_id| !def_id.is_local()));
+        vis.map_id(|def_id| {
+            assert!(!def_id.is_local());
+        });


I think all these asserts can be removed now.
define_extern is called only from build_reduced_graph_for_external_crate_res, so the confidence here is high.

And I think the other added asserts can be also removed now.

rustbot assigned petrochenkov Jul 13, 2025

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 13, 2025

LorrensP-2158466 commented Jul 13, 2025

View reviewed changes

compiler/rustc_resolve/src/build_reduced_graph.rs Outdated Show resolved Hide resolved

LorrensP-2158466 commented Jul 13, 2025

View reviewed changes

compiler/rustc_resolve/src/imports.rs Show resolved Hide resolved

LorrensP-2158466 commented Jul 13, 2025

View reviewed changes

compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved

LorrensP-2158466 commented Jul 13, 2025

View reviewed changes

compiler/rustc_resolve/src/imports.rs Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 14, 2025

petrochenkov reviewed Jul 14, 2025

View reviewed changes

compiler/rustc_resolve/src/build_reduced_graph.rs Outdated Show resolved Hide resolved

This was referenced Jul 16, 2025

resolve: Make disambiguators for underscore bindings module-local #144013

Merged

resolve: Use interior mutability for extern module map #143550

Merged

LorrensP-2158466 force-pushed the resolve-split-define branch from 8515b3c to 1296920 Compare July 17, 2025 15:57

This comment has been minimized.

Sign in to view

LorrensP-2158466 force-pushed the resolve-split-define branch from 89cf98b to 4497a13 Compare July 17, 2025 18:47

LorrensP-2158466 marked this pull request as ready for review July 17, 2025 19:57

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 17, 2025

LorrensP-2158466 changed the title ~~[WIP] Resolve: refactor define into define_local and define_extern~~ Resolve: refactor define into define_local and define_extern Jul 17, 2025

This comment was marked as resolved.

Sign in to view

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 18, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 24, 2025

update define_extern

05925fb

petrochenkov reviewed Jul 24, 2025

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2025

address review

a9d94be

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 24, 2025

petrochenkov reviewed Jul 25, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Jul 25, 2025

Auto merge of #143884 - LorrensP-2158466:resolve-split-define, r=<try>

afc668a

Resolve: refactor `define` into `define_local` and `define_extern`

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 25, 2025

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 25, 2025

This comment has been minimized.

Sign in to view

address review

a819395

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 25, 2025

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 25, 2025

petrochenkov reviewed Jul 25, 2025

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2025

	if resolution.non_glob_binding.replace(binding).is_some() {
	if self.resolution(parent, key).borrow_mut().non_glob_binding.replace(binding).is_some() {

Resolve: refactor define into define_local and define_extern #143884

Are you sure you want to change the base?

Resolve: refactor define into define_local and define_extern #143884

Conversation

LorrensP-2158466 commented Jul 13, 2025 • edited by petrochenkov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

LorrensP-2158466 commented Jul 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petrochenkov commented Jul 14, 2025

Uh oh!

Uh oh!

This comment has been minimized.

LorrensP-2158466 commented Jul 17, 2025

Uh oh!

This comment was marked as resolved.

LorrensP-2158466 commented Jul 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LorrensP-2158466 commented Jul 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Jul 25, 2025

Uh oh!

This comment has been minimized.

rust-bors bot commented Jul 25, 2025

Uh oh!

rust-bors bot commented Jul 25, 2025

Uh oh!

This comment has been minimized.

LorrensP-2158466 commented Jul 25, 2025

Uh oh!

rust-timer commented Jul 25, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Resolve: refactor `define` into `define_local` and `define_extern` #143884

Resolve: refactor `define` into `define_local` and `define_extern` #143884

LorrensP-2158466 commented Jul 13, 2025 •

edited by petrochenkov

Loading

LorrensP-2158466 commented Jul 13, 2025 •

edited

Loading